-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always use getDocument in FocusZone and FocusTrapZone #10187
Always use getDocument in FocusZone and FocusTrapZone #10187
Conversation
Merge latest from fabric-ui
merge to fork
Exceeds Tolerance Exceeds Baseline Below Baseline 1 kB = 1000 bytes |
Component Perf Analysis:
|
@@ -277,7 +280,8 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl | |||
} | |||
|
|||
if (FocusTrapZone._focusStack.length && this === FocusTrapZone._focusStack[FocusTrapZone._focusStack.length - 1]) { | |||
const focusedElement = document.activeElement as HTMLElement; | |||
const doc = getDocument(this._root.current); | |||
const focusedElement = doc!.activeElement as HTMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are enough uses here that it's worthwhile to make a little helper function and call it directly while eliminating the need for intermediate local doc
variables. Something like:
private getDocument() {
return getDocument(this._root.current);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. But still, make sense to remain in places were doc
variable is used multiple times within a function scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JasonGore addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't there more replaceable instances above? I only see once case where it's actually used more than once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced all document.*
usage with _getDocument().*
.In 2 places in FocusZone
and and 1 place in FocusTrapZone
I saved it to variable const doc = this._getDocument();
and used as doc.*
because it is used more than once within the function scope. Let me know if it does make sense
|
||
const doc = this._getDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is important, otherwise, SSR tests will fail
🎉 Handy links: |
Pull request checklist
$ yarn change
Description of changes
Use
getDocument
instead ofdocument
object to keep consistency and be less error-prone.Use case:
When rendering components (for e.g. FocusZone and FocusTrapZone) in an external window using
window.open
,document
object will be used incorrectly as it is taken from the parent window not from the current.A code example of
PortalWindow
when render child components therehttps://codesandbox.io/s/stardust-ui-example-1phxh
Microsoft Reviewers: Open in CodeFlow